Refactor WebViewActivity path handling#6447
Refactor WebViewActivity path handling#6447Gifford47 wants to merge 24 commits intohome-assistant:mainfrom
Conversation
|
@TimoPtr can you please have a look on it? you have additional webview PRs ... maybe there's conflict between the PRs ... |
TimoPtr
left a comment
There was a problem hiding this comment.
Thanks for your work, did you test it in multiple conditions? Like changing quickly the URL multiple times?
Also yes I'm currently working on making a new version of this WebViewActivity, it is going to take some time but you can already look at how it looks #6386
|
Also check what is happening when having arguments in the url like some filtered on the history page. |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Thanks for the thorough review @TimoPtr! I've addressed your feedback: Duplicate path retrieval: Removed getCurrentWebViewPath() from the Activity — the Presenter already handles this fallback in collectUrlStateChanges(), so the Activity now only passes the intentPath (or null). intent.removeExtra(EXTRA_PATH): Moved before presenter.load() so the extra is consumed immediately. moreInfoEntity assignment: Fixed the behavioral change — removed the unconditional raw assignment. Now only sets moreInfoEntity to the regex-validated entity for older servers (JS dispatch path), and explicitly clears it when using the URL path approach (>= 2025.6). Exception handling in getCurrentWebViewPath(): Removed the broad catch (Exception) — Uri.parse() is lenient and doesn't throw, so the try-catch was unnecessary. Path extraction simplification: Replaced with webView.url?.toUri()?.path?.takeIf { it.length > 1 } as suggested. Comment reference: Updated to use the full GitHub issue URL. Regarding URL arguments (e.g. filters on the history page): Uri.path only returns the path component and excludes query parameters, so filtered views like /history?entity_id=sensor.foo will correctly preserve just /history as the path — the frontend will reload with its default state for that view, which is the expected behavior when switching between internal/external URLs. |
Actually it might be nice to keep the whole URI and just change the host/port imagine you are on It would be nice to only change the host/port |
|
The issue would be that most probably you are going to loose the history and if it is not the case I wonder what happens. Did you try to play with the app to see how it behaves for the history? |
|
Changes
Added getCurrentWebViewRelativeUrl() which extracts the full relative URL (path + query parameters + fragment) from the current WebView URL, stripping the external_auth parameter since the presenter re-adds it on every load.
Added lastBaseUrl tracking in collectUrlStateChanges to detect when the base URL changes (internal ↔ external).
After history is cleared, rapid urlFlow emissions can create stale history entries from the old connection. The back button handler now validates the origin of the previous history entry using WebBackForwardList before navigating back. Tested: WiFi → mobile data switch on Lovelace tabs: URL switches correctly, page preserved |
|
@TimoPtr all tests are successfully done. Do you have any hints? |
| fun Uri.toRelativeUrl(excludeParams: Set<String> = emptySet()): String? { | ||
| val path = encodedPath?.takeIf { it.length > 1 } ?: return null | ||
|
|
||
| val relativeUrl = Uri.Builder() | ||
| .encodedPath(path) | ||
| .apply { | ||
| queryParameterNames | ||
| .filterNot { it in excludeParams } | ||
| .flatMap { name -> getQueryParameters(name).map { name to it } } | ||
| .forEach { (name, value) -> appendQueryParameter(name, value) } | ||
| } | ||
| .encodedFragment(encodedFragment) | ||
| .build() | ||
| .toString() |
There was a problem hiding this comment.
queryParameterNames is a Set, so iteration order isn’t guaranteed; this can make toRelativeUrl() output nondeterministic (and can cause flaky tests or inconsistent navigation URLs) when multiple query params exist. Consider preserving the original query order by parsing encodedQuery (filtering out excluded param names while keeping ordering/encoding), or at minimum sorting parameter names before appending to ensure deterministic output.
- BackAction: inline previousUrl lookup (TimoPtr suggestion) - WebViewPresenterImpl: simplify keepHistory expression with De Morgan - WebViewPresenterImpl: add unit test for base URL change path preservation - WebViewActivity: expand doUpdateVisitedHistory comment with predictive back rationale - WebViewBackNavigationTest: capitalize Given to match project convention - ktlint: rename WebViewBackNavigation.kt -> BackAction.kt, fix import order
- BackAction: make inner resolveBackAction private, move resolution KDoc to public function so it shows at call sites - WebViewPresenterImpl: gate baseUrlChanged path preservation with !isNewServer (Copilot bug: prevents leaking path across server switches) - WebViewPresenterImpl: simplify baseUrlChanged using lastBaseUrl nullability - WebViewBackNavigationTest: drop Robolectric in favor of mockk-based WebView mocks, exercise logic through the public resolveBackAction overload
- BackAction: guard against currentIndex == 0 in previousUrl lookup - WebViewActivity: drop redundant moreInfoEntity = "" reset in the URL-path branch; moreInfoEntity is never set in this branch, so clearing it is unnecessary
Did you already test the feature? On my side everything works as expected. I tested it for some days in my productive environment. |
I honestly didn't because I don't have a test setup with internal/external URL. I want to test this once it's merged because we have an internal lane on the play store. I focused on the code itself for now. |
|
The switching now mostly works from a technical point of view and preserves the URL when switching between networks on the same server, and doesn't when switching servers. However I have two issues:
Before: shortcut-pre.mp4After (this also shows the animation issue): shortcut-post.mp4(I'm using energy as an example as I'm not that much into dashboards myself, but you can imagine people having shortcuts to specific dashboards and/or views who get this extra step now.) |
…eToRoot guard Require previousUrl != null for BackAction.NavigateToRoot so that pressing back with an empty back-stack (e.g. opened via deeplink to /history) returns None instead of synthesizing a navigation to root. This aligns resolveBackAction with jpelgrom's concern about losing Android's predictive-back peek animation.
|
I've spent some time thinking about a solution for the keepHistory issue. Logic:
Fixes both points:
Touched files: new If the direction works for you I'll prepare the changes. |
I'm not a big fan to try to keep a secondary history. Especially that if you are deep into the app and switch URL each back would reload the app because the origin would be different, so showing the loader every back. I invite you to join the discord discussion with the product people to decide where to go with this. Since it's mostly a UX question not really technical. |
|
Even with the latest changes, predictive back doesn't work at all in my testing. Likely because the history doesn't start with My WebView has decided to start doing native crashes when changing network somewhat regularly, making testing quite inconvenient. It does seem that when it doesn't crash it is never inserting a navigation to the default dashboard anymore though. |
|
Pushed three commits addressing predictive-back end-to-end (both code paths):
Tested on Android 16 emulator (API 36.1), gesture navigation, both code paths: Legacy WebViewActivity (USE_FRONTEND_V2 = false): predictive-back works on root, normal webView.goBack() inside the app. |
The OnBackPressedCallback in WebViewActivity was kept enabled whenever the loaded URL had a non-root path, which caused Android to assume the app would handle the back gesture and suppress the predictive-back peek animation — even on the root screen with no browser history. Drop the hasNonRootPath() fallback in doUpdateVisitedHistory so the callback is only enabled when webView.canGoBack() is true. With the earlier resolveBackAction guard requiring previousUrl != null, NavigateToRoot is now produced only when the WebView genuinely has back-stack entries — so the fallback was redundant and only served to keep the callback armed against predictive-back.
Summary
This PR refactors how paths are handled in the WebView to ensure navigation keeps the current path and exposes it reliably.
Key Changes
WebViewPresenterImpl.kt,WebViewActivity.kt,WebView.kt.getCurrentPath()→ retrieves the active WebView path.Why
Impact
getCurrentPath()can be used wherever current WebView state is needed.Checklist
Screenshots
Link to pull request in documentation repositories
User Documentation: home-assistant/companion.home-assistant#
Developer Documentation: home-assistant/developers.home-assistant#
Any other notes
See issue #4983